Add a workaround for a possible null offset ID. by Isira-Seneviratne · Pull Request #5067 · TeamNewPipe/NewPipe

Isira-Seneviratne · 2020-12-02T04:15:26Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

Works around an issue reported by @opusforlife2 in which the ID of the UTC ZoneOffset might be null (even though its documentation explicitly says that a ZoneId's ID value is not null).

APK testing

app-release.zip

Due diligence

I read the contribution guidelines.

opusforlife2 · 2020-12-02T06:08:34Z

Debug builds always work (I tried this one as well just to be certain) so I need a release build to test, remember?

Isira-Seneviratne · 2020-12-02T07:36:43Z

Debug builds always work (I tried this one as well just to be certain) so I need a release build to test, remember?

Right, my bad. I'll upload one now.

opusforlife2 · 2020-12-02T09:09:52Z

Works! Fixed for real this time! (Hopefully xD )

Isira-Seneviratne · 2020-12-02T09:32:38Z

Works! Fixed for real this time! (Hopefully xD )

Maybe you could report the bug here?

opusforlife2 · 2020-12-03T06:42:22Z

It could be a bug in LineageOS 16.

XiangRongLin

I feel like a bug fix like this, which which does not contain any UI stuff, should be covered by a unit tests.
When the underlying bug is fixed and the workaround is not needed anymore, it provides an easy way to verify that it still behaves as expected.

XiangRongLin · 2020-12-03T12:34:41Z

app/src/main/java/org/schabi/newpipe/util/Localization.java

    // Math.addExact() and Math.multiplyExact() are desugared even though lint displays a warning.
    @SuppressWarnings("NewApi")
-    private static GregorianCalendar fromUTC(final OffsetDateTime offsetDateTime) {
+    public static GregorianCalendar fromUTC(final OffsetDateTime offsetDateTime) {


Don't make a private method public for the sake of testing.

Instead you should test the methods thats using the private method or extract the method into it's own class (not 100% sure on the second part myself yet)

In this case i would write tests for public static String relativeTime(final OffsetDateTime offsetDateTime). If done this way you will provide the value of having regression test for when the workaround is removed. Because currently your tests will need to be removed, when the workaround ist removed.

I tried creating an instrumented test, but attempting to run it results in an error message saying that the test class cannot be found.

I'll open an pr to your branch tonight/tomorrow with what i mean. Maybe i missed something, that makes what i mean not possible.

No, I get that you want to test Localization.relativeTime() directly, but I can't run the instrumented test, so I tested PrettyTime manually.

Ok, now i understand why you need an instrumented test. The dependency are direcly accessed and not injected. This would need some major refactoring, which can be done in another PR.

I created a PR with Dagger, so I could add a commit for DI with Localization there.

XiangRongLin · 2020-12-04T17:19:44Z

app/src/main/java/org/schabi/newpipe/util/Localization.java


    public static String relativeTime(final OffsetDateTime offsetDateTime) {
-        return relativeTime(GregorianCalendar.from(offsetDateTime.toZonedDateTime()));
+        return relativeTime(fromUTC(offsetDateTime));


~~The tests are not covering the bug that you fixed. I can change back this line and your tests still pass successfully~~

nvm, i realised my mistake.

Isira-Seneviratne · 2020-12-05T10:33:43Z

That's because it seems to be specific to @opusforlife2's device. On my device, for instance, `GregorianCalendar.from()` works fine.

…

On Fri, Dec 4, 2020, 10:50 PM XiangRongLin ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In app/src/main/java/org/schabi/newpipe/util/Localization.java <#5067 (comment)>: > @@ -314,13 +318,33 @@ private static void initPrettyTime(final Context context) { } public static String relativeTime(final OffsetDateTime offsetDateTime) { - return relativeTime(GregorianCalendar.from(offsetDateTime.toZonedDateTime())); + return relativeTime(fromUTC(offsetDateTime)); The tests are not covering the bug that you fixed. I can change back this line and your tests still pass successfully — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5067 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHMXFEWPDAJL624PK3YA5KTSTEK5NANCNFSM4UJ5YORA> .

Isira-Seneviratne · 2020-12-10T09:47:09Z

@opusforlife2 could you test the new APK? I moved the workaround to OffsetDateTime.toCalendar() so that this issue won't potentially affect FeedViewModel.

TobiGr

thanks again

opusforlife2 · 2020-12-12T03:32:36Z

@Isira-Seneviratne 0.20.6 works perfectly fine, so your modification seems to be okay.

Isira-Seneviratne mentioned this pull request Dec 2, 2020

Fixed crash when starting NewPipe after it was removed from RAM #4921

Merged

5 tasks

TobiGr added the bug Issue is related to a bug label Dec 2, 2020

XiangRongLin reviewed Dec 3, 2020

View reviewed changes

XiangRongLin requested changes Dec 3, 2020

View reviewed changes

Isira-Seneviratne force-pushed the Add_workaround_for_null_offset_ID branch from eeee9ef to 6c7a216 Compare December 3, 2020 14:07

XiangRongLin requested changes Dec 4, 2020

View reviewed changes

Isira-Seneviratne force-pushed the Add_workaround_for_null_offset_ID branch 3 times, most recently from b9138bc to ca30c7a Compare December 10, 2020 09:39

Add a workaround for a possible null offset ID.

c0d6c8a

Isira-Seneviratne force-pushed the Add_workaround_for_null_offset_ID branch from ca30c7a to c0d6c8a Compare December 10, 2020 09:53

TobiGr approved these changes Dec 10, 2020

View reviewed changes

TobiGr merged commit 3c306a0 into TeamNewPipe:dev Dec 10, 2020

Isira-Seneviratne deleted the Add_workaround_for_null_offset_ID branch December 10, 2020 10:39

Uh oh!

Conversation

Isira-Seneviratne commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is it?

Description of the changes in your PR

APK testing

Due diligence

Uh oh!

opusforlife2 commented Dec 2, 2020

Uh oh!

Isira-Seneviratne commented Dec 2, 2020

Uh oh!

opusforlife2 commented Dec 2, 2020

Uh oh!

Isira-Seneviratne commented Dec 2, 2020

Uh oh!

opusforlife2 commented Dec 3, 2020

Uh oh!

XiangRongLin left a comment

Choose a reason for hiding this comment

Uh oh!

XiangRongLin Dec 3, 2020

Choose a reason for hiding this comment

Uh oh!

Isira-Seneviratne Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

XiangRongLin Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

Isira-Seneviratne Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

XiangRongLin Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

Isira-Seneviratne Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

XiangRongLin Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Isira-Seneviratne commented Dec 5, 2020 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Isira-Seneviratne commented Dec 10, 2020

Uh oh!

TobiGr left a comment

Choose a reason for hiding this comment

Uh oh!

opusforlife2 commented Dec 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Isira-Seneviratne commented Dec 2, 2020 •

edited

Loading

XiangRongLin Dec 4, 2020 •

edited

Loading

Isira-Seneviratne commented Dec 5, 2020 via email •

edited

Loading